fix: take lock in PriorityJobQueue.is_done - #101
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesQueue completion synchronization
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/tests/test_queue.py`:
- Around line 367-375: Fix the TOCTOU race in the monitor function by
re-checking queue.is_done after obtaining the get_all_jobs() snapshot, and only
recording “is_done True with non-terminal jobs” when is_done remains true.
Preserve the existing empty-queue check and terminal-status validation, while
avoiding false violations caused by jobs enqueued between the two calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 303a4c71-68e0-4ab8-a538-c5f61b12d85f
📒 Files selected for processing (3)
CHANGELOG.mdcli/localci/core/queue.pycli/tests/test_queue.py
|
queue.py:233 - move WAITING_DEPS jobs back to QUEUED once their dependencies finish, the way line 339 already does for WAITING_PRIORITY (nothing ever moves a job out of WAITING_DEPS. Two jobs linked by needs land at the same priority by default, so the dependent is stranded, the level never completes, and _dispatch_loop spins forever) pre-existing |
| t.join() | ||
| assert queue.total_jobs == 100 | ||
|
|
||
| def test_concurrent_is_done_invariant(self): |
There was a problem hiding this comment.
test_queue.py:356, CHANGELOG.md:28 - the test is not a regression guard.
Give it a failure mode or rename it, and reword the changelog as a consistency fix rather than a fixed race
(with the lock reverted it passed 20 out of 20 runs across 699 observations of is_done == True.
The unlocked read can only go stale in the harmless direction, so a future revert goes unnoticed)
| with violations_lock: | ||
| violations.append(msg) | ||
|
|
||
| def monitor() -> None: |
There was a problem hiding this comment.
read is_done and the job statuses in one locked call, instead of calling is_done and then inspecting what get_all_jobs returned
(get_all_jobs hands back live job objects, so the monitor checks status after the lock is gone and any transient has already healed. Only worth doing if the test can fail at all, since it means adding a production accessor for one test)
| stop.set() | ||
| monitor_thread.join() | ||
|
|
||
| assert not violations, violations |
There was a problem hiding this comment.
assert total_jobs == 100 and passed_count == 100, and have the monitor count it's samples with an assertion
That the count is above zero
(neither assertion pins the job count or shows thee monitor ran , and assert queue.js_done is Trus just restates the consumer loop's own exit condition, so a dead producer leaves the test green)
| queue.enqueue(make_job(f"Job {i}", priority=1, index=i)) | ||
| time.sleep(0.001) | ||
|
|
||
| def consume() -> None: |
There was a problem hiding this comment.
put a deadline on the consumer loop, give every join a timeout, and set timeout-minutes on the ci.yml test job
(the loop exits only when is_done goes true and the joins are unbounded. pytest-timeout is not installed and only the integration job sets a timeout, so finding 1 burns the 6-hour Actions default on three Python versions instead of failing)
2 participants
Closes #100
Summary
PriorityJobQueue.is_donenow acquiresself._lockwhen reading_jobsand_completed_keys, matchingis_emptyand other accessorstest_concurrent_is_done_invariantto stress concurrent enqueue/complete while a monitor thread checks thatis_doneis onlyTruewhen every job is terminal[Unreleased]->### FixedTest plan
pytest tests/test_queue.py::TestThreadSafety -vpytest -m "not integration"(full unit suite)pre-commit run -aNotes
completed_count,running_count, andtotal_jobsremain unlocked; left for a follow-up per issue scope.Summary by CodeRabbit
Bug Fixes
Tests
Documentation